Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Better Dynamic converter #78

Merged
merged 16 commits into from
Feb 28, 2022
Merged

Better Dynamic converter #78

merged 16 commits into from
Feb 28, 2022

Conversation

w1ndy
Copy link
Contributor

@w1ndy w1ndy commented Feb 23, 2022

Pull Request Description

This PR improves the conversion of Dynamic into different types of values. Major changes includes:

  • Support the conversion of Dynamic into slices (as far as I know there is no way to do this with the latest release)
  • Support the conversion of Dynamic into map[string]struct
  • Reuse code for converting Dynamic into pointer values
  • Add tests

Future Release Comment

[Add description of your change, to include in the next release]
[Delete any or all irrelevant sections, e.g. if your change does not warrant a release comment at all]

Fixes:

  • Support the conversion of Dynamic into slices

@codecov-commenter
Copy link

Codecov Report

Merging #78 (502f43f) into master (ca7cd57) will increase coverage by 0.83%.
The diff coverage is 65.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   47.20%   48.04%   +0.83%     
==========================================
  Files          39       39              
  Lines        4226     4196      -30     
==========================================
+ Hits         1995     2016      +21     
+ Misses       2086     2032      -54     
- Partials      145      148       +3
Impacted Files Coverage Δ
kusto/data/value/dynamic.go 61.42% <65.21%> (+42.42%) ⬆️
kusto/test/etoe/etoe_env.go 42.00% <0.00%> (-6.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca7cd57...502f43f. Read the comment docs.

@AsafMah
Copy link
Contributor

AsafMah commented Feb 28, 2022

Great job!

I hope you won't mind that I did some work on the tests - modernized them and added negative tests.

@AsafMah AsafMah self-requested a review February 28, 2022 07:36
@github-actions
Copy link

github-actions bot commented Feb 28, 2022 •
edited
Loading

Unit Test Results

    1 files  ±  0    13 suites  ±0   9s ⏱️ - 11m 42s
125 tests +15  112 ✔️ +7  13 💤 +13  0 ❌  - 5 

Results for commit 98c6e81. ± Comparison against base commit 75e351f.

This pull request removes 27 and adds 42 tests. Note that renamed tests count towards both. 
etoe ‑ TestFileIngestion/Ingest_from_blob_with_bad_existing_mapping
etoe ‑ TestFileIngestion/Ingest_from_blob_with_existing_mapping
etoe ‑ TestFileIngestion/Ingest_from_blob_with_inline_mapping
etoe ‑ TestFileIngestion/Ingest_from_blob_with_streaming_ingestion_should_fail
etoe ‑ TestFileIngestion/Ingest_from_local_with_existing_mapping_streaming
etoe ‑ TestFileIngestion/Ingestion_from_local_file_queued
etoe ‑ TestFileIngestion/Ingestion_from_local_file_streaming
etoe ‑ TestFileIngestion/Ingestion_from_local_file_test_2_queued
etoe ‑ TestFileIngestion/Ingestion_from_local_file_test_2_streaming
etoe ‑ TestMultipleClusters/Ingestion_from_local_file_streaming
…

table ‑ TestFieldsConvert/non-valid_Bool
table ‑ TestFieldsConvert/non-valid_DateTime
table ‑ TestFieldsConvert/non-valid_Dynamic
table ‑ TestFieldsConvert/non-valid_Dynamic_list
table ‑ TestFieldsConvert/non-valid_GUID
table ‑ TestFieldsConvert/non-valid_Int
table ‑ TestFieldsConvert/non-valid_Long
table ‑ TestFieldsConvert/non-valid_String
table ‑ TestFieldsConvert/non-valid_Timespan
table ‑ TestFieldsConvert/non-valid_real
…

♻️ This comment has been updated with latest results.

@AsafMah AsafMah merged commit e6123c7 into Azure:master Feb 28, 2022
@w1ndy
Copy link
Contributor Author

w1ndy commented Mar 1, 2022

@AsafMah Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AsafMah AsafMah AsafMah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@w1ndy @codecov-commenter @AsafMah